Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix test failure and delete unused code #60747

Merged
merged 1 commit into from
Mar 6, 2018
Merged

Fix test failure and delete unused code #60747

merged 1 commit into from
Mar 6, 2018

Conversation

dims
Copy link
Member

@dims dims commented Mar 3, 2018 •
edited

What this PR does / why we need it:
Daemon set [Serial] should update pod when spec was updated and update strategy is RollingUpdate is broken by recent updates as Template generation isn't supported by apps/v1.DaemonSet anymore

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #60745

Special notes for your reviewer:

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 3, 2018
@dims dims mentioned this pull request Mar 3, 2018
Closed
@janetkuo janetkuo self-assigned this Mar 3, 2018
@dims dims force-pushed the fix-daemonset-conformance-test-failure-and-remove-unused-code branch from 0c472ae to d99f24f Compare March 4, 2018 17:29
@dims dims changed the title [WIP] fix test failure and delete unused code Fix test failure and delete unused code Mar 4, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2018
@kow3ns kow3ns added this to Backlog in Workloads via automation Mar 4, 2018
@kow3ns kow3ns moved this from Backlog to In Progress in Workloads Mar 4, 2018
@dims
Copy link
Member Author

dims commented Mar 4, 2018

/test pull-kubernetes-local-e2e

@timothysc
Copy link
Member

There is no issue specified, or link to broken test(s).

@dims dims changed the title Fix test failure and delete unused code [WIP Fix test failure and delete unused code Mar 5, 2018
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2018
@dims dims changed the title [WIP Fix test failure and delete unused code [WIP] Fix test failure and delete unused code (ONLY for testing pull CI job) Mar 5, 2018
@dims
Copy link
Member Author

dims commented Mar 5, 2018 •
edited by janetkuo

@timothysc please see #60745. i have updated the title to reflect what i am trying to do here (test pull job)

@dims
Copy link
Member Author

dims commented Mar 5, 2018

Looks like my patch fixes the problem in the test (using local-up-cluster) - http://paste.openstack.org/show/692060/

@dims
Copy link
Member Author

dims commented Mar 5, 2018

/test pull-kubernetes-local-e2e

janetkuo
janetkuo approved these changes Mar 5, 2018
View reviewed changes
Copy link
Member

@janetkuo janetkuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this fix ready? It looks good to me. Just a nit on log.

test/e2e/apps/daemon_set.go
@@ -315,8 +314,6 @@ var _ = SIGDescribe("Daemon set [Serial]", func() {
framework.ConformanceIt("should update pod when spec was updated and update strategy is RollingUpdate", func() {
label := map[string]string{daemonsetNameLabel: dsName}

templateGeneration := int64(999)
framework.Logf("Creating simple daemon set %s with templateGeneration %d", dsName, templateGeneration)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add framework.Logf("Creating simple daemon set %s", dsName)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janetkuo ack, will fix it up in a minute

@janetkuo
Copy link
Member

janetkuo commented Mar 5, 2018

Template generation isn't supported by apps/v1.DaemonSet anymore. The behavior is still preserved in DaemonSet controller for backward compatibility (users can still use it with extensions/v1beta1 endpoint).

#59883 moved DaemonSet tests to apps/v1 but didn't remove the Template Generation part, which caused the failure this PR tries to fix.

We should still test template generation to make sure we don't break backward compatibility. That can be done in integration test. cc @crimsonfaith91

@dims
fix test failure and delete unused code
4cbf397 
Removing the template generation that was left behind when the API was
updated in 22fb5c4

Also cleaned up many unused methods left behind
@dims dims force-pushed the fix-daemonset-conformance-test-failure-and-remove-unused-code branch from d99f24f to 4cbf397 Compare March 5, 2018 22:15
@dims dims changed the title [WIP] Fix test failure and delete unused code (ONLY for testing pull CI job) Fix test failure and delete unused code Mar 5, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2018
@crimsonfaith91
Copy link
Contributor

@janetkuo Thank you for the information!

@janetkuo janetkuo added this to the v1.10 milestone Mar 5, 2018
@janetkuo janetkuo added queue/fix kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. and removed milestone/incomplete-labels labels Mar 5, 2018
@dims
Copy link
Member Author

dims commented Mar 5, 2018

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Mar 5, 2018
@janetkuo janetkuo added sig/apps Categorizes an issue or PR as relevant to SIG Apps. kind/bug Categorizes issue or PR as related to a bug. labels Mar 5, 2018
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@dims @janetkuo

Pull Request Labels
  • sig/apps: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/bug: Fixes a bug discovered during the current release.
Help

@janetkuo janetkuo mentioned this pull request Mar 5, 2018
Closed
@janetkuo
Copy link
Member

janetkuo commented Mar 5, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, janetkuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@dims
Copy link
Member Author

dims commented Mar 6, 2018

/test pull-kubernetes-e2e-gce

1 similar comment
@dims
Copy link
Member Author

dims commented Mar 6, 2018

/test pull-kubernetes-e2e-gce

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 6, 2018 •
edited

@dims: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e d99f24f599b6a03bfd25b122bce9f8c9bbad1982 link /test pull-kubernetes-local-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dims
Copy link
Member Author

dims commented Mar 6, 2018

/test pull-kubernetes-e2e-gce

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 48a7048 into kubernetes:master Mar 6, 2018
Workloads automation moved this from In Progress to Done Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janetkuo janetkuo janetkuo approved these changes

@tnozicka tnozicka Awaiting requested review from tnozicka

Assignees

@janetkuo janetkuo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Workloads
  
Done
Milestone
v1.10
7 participants
@dims @timothysc @janetkuo @crimsonfaith91 @k8s-github-robot @k8s-ci-robot @fejta-bot